Adds get flows by product#65
Conversation
- Implement `WriteJSONResponse` utility to encode data to JSON and write HTTP responses with improved reliability. - Add unit tests to cover success, error, and content-type validation scenarios.
…Product` with tests
… for cleaner response handling
- Define HTTP GET request to fetch flows for a specific product using its ID. - Include request structure, example response, and response validation.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements the ChangesGet Flows by Product Endpoint Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@_bruno/Flows/Get` Flows by Product.yml:
- Around line 53-58: The example response for the "Get Flows by Product" flow is
missing the product_id field; update the sample JSON object (the Flow example
with keys id, name, description, created_at, updated_at) to include the
product_id property with an appropriate sample value so the documented response
shape matches the endpoint contract.
In `@internal/flow/handler_test.go`:
- Around line 315-323: The test case named "No Flows Found" in
internal/flow/handler_test.go is asserting a 404 but the handler should return
200 with an empty array when the product exists but has no flows; update that
case so mockFlowQuerier.getFlowsByProductFunc returns (nil, nil) and
expectedStatus is http.StatusOK and assert an empty JSON array response. Also
add a new case (e.g., "Product Not Found") where
mockFlowQuerier.getFlowsByProductFunc returns (nil, pgx.ErrNoRows) and
expectedStatus is http.StatusNotFound to cover the missing-product contract.
In `@internal/flow/handler.go`:
- Around line 88-90: The handler currently treats pgx.ErrNoRows as "product not
found" and returns 404; instead distinguish the two cases by changing the data
layer/query and the handler: have the data function (e.g., GetFlowsByProduct)
return an explicit sentinel error ErrProductNotFound when the product row itself
is missing, and return an empty slice (and nil error) when the product exists
but has zero flows; then update the handler to check for errors.Is(err,
ErrProductNotFound) to call internal.HandleHttpError(..., http.StatusNotFound)
and otherwise treat a nil error with an empty slice as a 200 + [] response,
removing the current errors.Is(err, pgx.ErrNoRows) 404 branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e0b6e2a-e50a-43cb-8bd5-6784ddb77e7c
📒 Files selected for processing (9)
_bruno/Flows/Get Flows by Product.ymlinternal/flow/handler.gointernal/flow/handler_test.gointernal/flow/interface.gointernal/platform/handler.gointernal/product/handler.gointernal/response.gointernal/response_test.gorouter/router.go
…andling - Introduce `service` struct in `flow` package for business logic: `CreateFlow`, `GetFlowById`, and `GetFlowsByProduct`. - Update handlers to delegate flow operations to `service` and improve separation of concerns. - Add `NotFoundError` for standardized error handling in case of missing flows or products. - Update SQLC query for `GetFlowsByProduct` to include `product_id` in the result. - Add `GetProductById` query to validate product existence before fetching related flows.
- Added tests for `CreateFlow`, `GetFlowById`, and `GetFlowsByProduct` methods in the `service` layer. - Updated `CreateFlow` and `GetFlowById` handlers to handle `NotFoundError` and validation errors consistently. - Enhanced mock implementation to accommodate new service methods and error scenarios.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
internal/flow/handler_test.go (1)
304-311: ⚡ Quick winAssert
product_idin the success payload.The success case only checks
len(flows), so it would still pass ifproduct_iddisappeared from the response. That field is part of the endpoint contract and one of the key query changes in this PR.Suggested test tightening
m.getFlowsByProductFunc = func(ctx context.Context, productID int) ([]Flow, error) { if productID != 1 { return nil, errors.New("unexpected product id") } return []Flow{ - {ID: 1, Name: "Flow 1"}, - {ID: 2, Name: "Flow 2"}, + {ID: 1, ProductID: 1, Name: "Flow 1"}, + {ID: 2, ProductID: 1, Name: "Flow 2"}, }, nil } @@ if tt.expectedStatus == http.StatusOK { var flows []Flow if err := json.NewDecoder(rr.Body).Decode(&flows); err != nil { t.Fatalf("failed to decode response: %v", err) } if len(flows) != 2 { t.Errorf("expected 2 flows, got %v", len(flows)) } + for _, flow := range flows { + if flow.ProductID != 1 { + t.Errorf("expected ProductID 1, got %v", flow.ProductID) + } + } if rr.Header().Get("Content-Type") != "application/json" { t.Errorf("expected Content-Type application/json, got %v", rr.Header().Get("Content-Type")) } }As per coding guidelines,
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".Also applies to: 365-372
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flow/handler_test.go` around lines 304 - 311, The test's success assertions only check len(flows) and omit verifying the response includes the product_id; update the tests that use m.getFlowsByProductFunc (the success cases around the existing blocks returning []Flow) to decode the handler response JSON and assert that the top-level "product_id" equals the requested product (e.g., 1). Locate the test cases referencing m.getFlowsByProductFunc and Flow, parse the response body into the expected response struct or map, and add an assertion for product_id in both success scenarios (the blocks around the current getFlowsByProductFunc stubs, including the other case around lines 365-372).internal/flow/service_test.go (1)
92-150: ⚡ Quick winAdd test cases for untested service branches introduced in this PR.
Current tests miss: (1) generic DB error in
GetFlowById, (2) generic DB error fromGetProductById, (3) generic DB error fromGetFlowsByProduct, and (4) product exists with no flows (expect empty slice contract).As per coding guidelines, "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request".
Also applies to: 152-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flow/service_test.go` around lines 92 - 150, The tests for the service are missing coverage for several error and edge branches: add unit tests that use the mockFlowQuerier to simulate (1) a generic DB error returned from GetFlowById (not pgx.ErrNoRows) and assert the propagated error, (2) a generic DB error from GetProductById and assert propagation when calling the service method that depends on product lookup, (3) a generic DB error from GetFlowsByProduct and assert the service surfaces that error, and (4) a case where a product exists but GetFlowsByProduct returns an empty slice — assert the service returns an empty slice (not nil) to preserve the contract; implement these by adding table-driven test cases similar to TestService_GetFlowById that set mockFlowQuerier.getFlowFunc/getProductFunc/getFlowsByProductFunc to return the desired errors or empty slice and verify expected error messages or empty slice results.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/flow/service.go`:
- Around line 47-51: The GetFlowsByProduct return can propagate a nil []Flow
which JSON-encodes to null; after calling s.queries.GetFlowsByProduct(ctx, id)
and before returning, ensure the returned flows slice is normalized to an empty
slice when nil (e.g., if flows == nil then set flows = make([]Flow, 0)) so the
method (in service.go) always returns an empty array instead of null while
preserving the existing error handling around s.queries.GetFlowsByProduct.
- Around line 20-23: The current error branch checks errors.Is(err,
pgx.ErrNoRows) but INSERT ... RETURNING foreign-key failures surface as
*pgconn.PgError with SQLSTATE "23503", so replace the pgx.ErrNoRows check with
logic that type-asserts err to *pgconn.PgError and, when pgErr.Code == "23503"
(optionally also checking pgErr.Constraint or Detail for the product FK name),
return Flow{}, internal.NewNotFoundError(id, "Product"); otherwise fall back to
the existing fmt.Errorf("failed to create flow: %w", err) handling. Ensure you
import github.com/jackc/pgconn if not already.
In `@internal/response_test.go`:
- Around line 76-79: The test branch for encoding failures currently only checks
status and Content-Type; update the case where tt.expectedStatus ==
http.StatusInternalServerError to also decode the response body (w.Body.Bytes())
as JSON and assert the problem+json payload contains a "detail" field with the
expected error message (or at least that it contains text indicating an encoding
failure) so the contract produced by HandleHttpError is validated; use the same
test table entry's expected message or a concrete substring and reference w,
tt.expectedStatus, and HandleHttpError/response writer usage to locate where to
add the JSON decode and assertion.
---
Nitpick comments:
In `@internal/flow/handler_test.go`:
- Around line 304-311: The test's success assertions only check len(flows) and
omit verifying the response includes the product_id; update the tests that use
m.getFlowsByProductFunc (the success cases around the existing blocks returning
[]Flow) to decode the handler response JSON and assert that the top-level
"product_id" equals the requested product (e.g., 1). Locate the test cases
referencing m.getFlowsByProductFunc and Flow, parse the response body into the
expected response struct or map, and add an assertion for product_id in both
success scenarios (the blocks around the current getFlowsByProductFunc stubs,
including the other case around lines 365-372).
In `@internal/flow/service_test.go`:
- Around line 92-150: The tests for the service are missing coverage for several
error and edge branches: add unit tests that use the mockFlowQuerier to simulate
(1) a generic DB error returned from GetFlowById (not pgx.ErrNoRows) and assert
the propagated error, (2) a generic DB error from GetProductById and assert
propagation when calling the service method that depends on product lookup, (3)
a generic DB error from GetFlowsByProduct and assert the service surfaces that
error, and (4) a case where a product exists but GetFlowsByProduct returns an
empty slice — assert the service returns an empty slice (not nil) to preserve
the contract; implement these by adding table-driven test cases similar to
TestService_GetFlowById that set
mockFlowQuerier.getFlowFunc/getProductFunc/getFlowsByProductFunc to return the
desired errors or empty slice and verify expected error messages or empty slice
results.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 499f24c7-e7b6-48e8-af0b-6e0375822ff4
📒 Files selected for processing (9)
internal/flow/flows.sqlinternal/flow/flows.sql.gen.gointernal/flow/handler.gointernal/flow/handler_test.gointernal/flow/querier.gen.gointernal/flow/service.gointernal/flow/service_test.gointernal/response.gointernal/response_test.go
- Move generated SQLC files from `internal/flow` to `internal/flow/db` for better organization. - Update imports and type references in the `flow` service, handlers, and tests to use `db` package. - Adjust mock implementations and test cases to match new package structure.
- Change SQLC output path from `internal/flow` to `internal/flow/db` for better organization and alignment with package structure.
- Introduce `expectedCount` field to test cases in `handler_test.go` for flexible flow count validation. - Add a new test case for "No Flows Found" scenario. - Refactor assertions to use `expectedCount` for improved test readability and accuracy.
- Add tests for "No Flows Found" and database error scenarios in `GetFlowsByProduct`. - Update `GetFlowsByProduct` service method to handle `pgx.ErrNoRows` gracefully by returning an empty slice.
…o use correct parameter name
…test case - Update `GetFlowsByProduct` service method to return an empty slice instead of nil when no flows are found. - Add a test case for "No Flows Found (Nil guard)" scenario.
- Validate error response decoding and correctness of the `Detail` field in test cases.
Description
Code Rabbit Summary
Summary by CodeRabbit
Release Notes
New Features
GET /api/products/{id}/flowsto retrieve flows by product ID.Bug Fixes
Refactor
Fixes
Closes #36
Post Deployment Tasks?